add restriction support like eu-access restriction #245

neo-liang-sap · 2020-08-04T01:45:30Z

What this PR does / why we need it:
This PR will display eu-access restriction warning msg in gardenctl terminal if cluster is configed with eu-access and garden config is configed with enabling the eu-access
Which issue(s) this PR fixes:
Fixes #232

Special notes for your reviewer:
To use this PR:

shoot needs to be configed with eu-access restriction
corresponding garden config needs to be config to enable the eu-access restriction
tests has been perform for following scenario with cross permutation
shoot:
a. config eu-access
b. config node-eu-access
c. addons eu-access
garden config:
a. has/has no access restriction
b. true/false in access restriction key and option key

Release note:

display eu-access restriction warning msg in gardenctl terminal if cluster is configed with eu-access and garden config is configed with enabling the eu-access restriction

neo-liang-sap · 2020-08-04T01:46:02Z

/cc @petersutter

neo-liang-sap · 2020-08-06T02:03:16Z

@gardener/gardenctl-maintainers any comments on this PR?

neo-liang-sap · 2020-08-07T02:04:42Z

per communication with @petersutter , he will review this PR after back from holiday

petersutter

overall lgtm, however to improve the readability of the code I have made some suggestions

petersutter · 2020-08-18T16:34:50Z

pkg/cmd/target.go

+	config := reader.ReadConfig(pathGardenConfig)
+	for _, garden := range config.GardenClusters {
+		if garden.Name == gardenName && len(garden.AccessRestrictions) > 0 {
+			ars = garden.AccessRestrictions


if you have a match you can return already here or at least break the loop

petersutter · 2020-08-18T16:41:27Z

pkg/cmd/target.go

@@ -1011,6 +1068,11 @@ func shootWrapper(targetReader TargetReader, targetWriter TargetWriter, configRe
 		return fmt.Errorf("no match for %q", args[1])
 	} else if len(shoots) == 1 {
 		targetShoot(targetWriter, shoots[0])
+		warningMsg := checkShootsRestriction(shoots[0], configReader, target.Stack()[0].Name)


extract target.Stack()[0].Name to variable for better readability

petersutter · 2020-08-18T16:48:06Z

pkg/cmd/target.go

+	for j := 0; j < len(ars); j++ {
+		ar := ars[j]


use range loop if you do not need the index

petersutter · 2020-08-18T16:58:46Z

pkg/cmd/target.go

+					for i := 0; i < len(ar.Options); i++ {
+						option := ar.Options[i]


same here, use range loop

petersutter · 2020-08-18T17:04:57Z

pkg/cmd/target.go

+		if _, ok := shootMatchLabels[ar.Key]; ok {
+			if shootMatchLabels[ar.Key] == strconv.FormatBool(ar.NotifyIf) {
+				warningMsg += ar.Msg
+				warningMsg += "\n"
+				//if upper level msg will not show, neither will lower level msg show
+				if len(ar.Options) > 0 {
+					for i := 0; i < len(ar.Options); i++ {
+						option := ar.Options[i]
+						if _, ok := shootAnnotations[option.Key]; ok {
+							if shootAnnotations[option.Key] == strconv.FormatBool(option.NotifyIf) {
+								warningMsg += option.Msg
+								warningMsg += "\n"
+							}
+						}
+
+					}
+				}
+
+			}
+		}


to simplify nested if statements, use early return pattern. This improves readability

neo-liang-sap · 2020-08-19T02:18:52Z

Hello @petersutter , i've fixed my PR per your comments, thanks for your review! -Neo

petersutter · 2020-08-19T07:28:33Z

OK I have just tested it locally. However it only worked when targeting the shoot via the dashboard url, e.g. gardenctl target --server https://gardener.example.com --project myproject --shoot myshoot but not via gardenctl target shoot myshoot
sorry my fault, it works now as expected

petersutter · 2020-08-19T11:45:57Z

I was using the command wrong for targeting a shoot by missing the shoot parameter. But somehow it still worked to target the shoot, except the warning was not displayed.
Why is this working anyhow? Is this intended?

bash-5.0$ ./gardenctl-darwin-amd64 target deleteme
Shoot:
KUBECONFIG=/Users/d050337/.garden/cache/dev-virtual/projects/core/deleteme/kubeconfig.yaml
                                                                                                                         
bash-5.0$ ./gardenctl-darwin-amd64 target shoot deleteme
Shoot:
KUBECONFIG=/Users/d050337/.garden/cache/dev-virtual/projects/core/deleteme/kubeconfig.yaml
Attention, EU-Access is enabled!
Do not debug cluster addons!

neo-liang-sap · 2020-08-19T11:52:31Z

Interesting. I will check the code and update here. Thanks!

neo-liang-sap · 2020-08-19T12:17:58Z

Hi @petersutter , thanks for your catch! yes gardenctl target xxx is by design and the code is here
i've update code to cover this scenario.
Thanks again for your nice catch!
-Neo

petersutter

/lgtm

neo-liang-sap requested a review from a team as a code owner August 4, 2020 01:45

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 4, 2020

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 4, 2020

neo-liang-sap force-pushed the support-access-restrictions branch from 2319d05 to 11cebfa Compare August 4, 2020 01:53

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 4, 2020

neo-liang-sap force-pushed the support-access-restrictions branch from 11cebfa to 5452952 Compare August 4, 2020 02:02

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 4, 2020

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 4, 2020

petersutter reviewed Aug 18, 2020

View reviewed changes

neo-liang-sap force-pushed the support-access-restrictions branch from 5452952 to a2367d2 Compare August 19, 2020 01:59

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 19, 2020

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 19, 2020

add restriction support like eu-access restriction

756ebfb

neo-liang-sap force-pushed the support-access-restrictions branch from a2367d2 to 756ebfb Compare August 19, 2020 12:14

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 19, 2020

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 19, 2020

petersutter self-requested a review August 19, 2020 13:18

petersutter approved these changes Aug 19, 2020

View reviewed changes

gardener-robot added the reviewed/lgtm Has approval for merging label Aug 19, 2020

neo-liang-sap merged commit 62c787c into gardener-attic:master Aug 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add restriction support like eu-access restriction #245

add restriction support like eu-access restriction #245

neo-liang-sap commented Aug 4, 2020

neo-liang-sap commented Aug 4, 2020

neo-liang-sap commented Aug 6, 2020

neo-liang-sap commented Aug 7, 2020

petersutter left a comment

petersutter Aug 18, 2020

petersutter Aug 18, 2020

petersutter Aug 18, 2020

petersutter Aug 18, 2020

petersutter Aug 18, 2020

neo-liang-sap commented Aug 19, 2020

petersutter commented Aug 19, 2020 •

edited

Loading

petersutter commented Aug 19, 2020

neo-liang-sap commented Aug 19, 2020

neo-liang-sap commented Aug 19, 2020

petersutter left a comment

		for i := 0; i < len(ar.Options); i++ {
		option := ar.Options[i]

add restriction support like eu-access restriction #245

add restriction support like eu-access restriction #245

Conversation

neo-liang-sap commented Aug 4, 2020

neo-liang-sap commented Aug 4, 2020

neo-liang-sap commented Aug 6, 2020

neo-liang-sap commented Aug 7, 2020

petersutter left a comment

Choose a reason for hiding this comment

petersutter Aug 18, 2020

Choose a reason for hiding this comment

petersutter Aug 18, 2020

Choose a reason for hiding this comment

petersutter Aug 18, 2020

Choose a reason for hiding this comment

petersutter Aug 18, 2020

Choose a reason for hiding this comment

petersutter Aug 18, 2020

Choose a reason for hiding this comment

neo-liang-sap commented Aug 19, 2020

petersutter commented Aug 19, 2020 • edited Loading

petersutter commented Aug 19, 2020

neo-liang-sap commented Aug 19, 2020

neo-liang-sap commented Aug 19, 2020

petersutter left a comment

Choose a reason for hiding this comment

petersutter commented Aug 19, 2020 •

edited

Loading